Add auto-ping feature to client#563
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aidangarske
left a comment
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: REQUEST_CHANGES
Findings: 3 total — 3 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
1 finding(s) not tied to a diff line (full detail below)
Posted findings
- [Medium] [review] Missing non-blocking auto-ping timeout coverage —
tests/test_mqtt_client.c:2023-2402 - [Low] [bugs] Auto-ping tests assume the time source is already past startup —
tests/test_mqtt_client.c:2045-2124
Findings not tied to a diff line
Non-blocking examples swallow unanswered auto-ping timeouts
File: examples/nbclient/nbclient.c:526-538, examples/mqttclient/mqttclient.c:600-616
Function: mqttclient_test
Severity: High
Category: Liveness / Error Handling
With WOLFMQTT_NONBLOCK and the new auto-ping enabled, MqttClient_WaitMessage can start client->keep_alive_ping and return MQTT_CODE_CONTINUE while waiting for PINGRESP. These examples then let mqtt_check_timeout convert repeated CONTINUE returns into MQTT_CODE_ERROR_TIMEOUT, but the new non-WOLFMQTT_NO_TIME branch treats that timeout as ordinary idle and continues. That loses the application's liveness deadline, which the PR says remains required under WOLFMQTT_NONBLOCK, so an unanswered auto PINGREQ can loop forever instead of disconnecting or surfacing an error.
Recommendation: Restore non-blocking liveness handling before merge. In WOLFMQTT_NONBLOCK builds, distinguish a plain idle poll from an auto-ping already in progress: if client->keep_alive_ping is active when mqtt_check_timeout fires, cancel or disconnect and return a real error; only swallow the timeout when no keep-alive exchange is pending.
Referenced code: examples/nbclient/nbclient.c:526-538, examples/mqttclient/mqttclient.c:600-611 (12 lines)
Review generated by Skoll
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #563
Scan targets checked: wolfmqtt-bugs, wolfmqtt-src
No new issues found in the changed files. ✅
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #563
Scan targets checked: wolfmqtt-bugs, wolfmqtt-src
No new issues found in the changed files. ✅
The core client now tracks outbound idle time and sends a PINGREQ from MqttClient_WaitMessage once the link has been idle for about three quarters of the negotiated keep-alive, so applications no longer have to schedule pings themselves. The time source is the compile-time macro WOLFMQTT_GET_TIME_S() (defaults to time(NULL), overridable; define WOLFMQTT_NO_TIME to compile it out), mirroring the broker. A v5 CONNACK Server Keep Alive override is honored, the scheduler is disarmed on disconnect, the mqttclient example keeps its old manual loop under WOLFMQTT_NO_TIME, and unit tests cover the scheduling paths.
Fixes #501